Fix 401/403 apiserver errors do not return 'Status' objects #47384

shiywang · 2017-06-13T00:25:23Z

sttts · 2017-06-13T08:22:48Z

@shiywang looks good. Can you add tests for both filter cases? Just add them below https://github.com/kubernetes/kubernetes/blob/ef7d63818fab7aeb3d356b46d3b0dddfd524b9c3/test/integration/master/master_test.go#L114-L114.

shiywang · 2017-06-14T08:06:08Z

@sttts testcase added, I also add another filter cases, but seems one unit test failed due to here 0025b07#diff-6445bd63d178373b50fc083b9a043d0bR72
do you think I should change this Authenticate filter at all ?

sttts · 2017-06-14T08:40:52Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go

+					return
+				}
+
+				gv := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}


would move this below the if

sttts · 2017-06-14T08:43:56Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go

 		glog.V(4).Infof("Forbidden: %#v, Reason: %q", req.RequestURI, reason)
-		responsewriters.Forbidden(attributes, w, req, reason)


would it be feasible to modify responsewriters.Forbidden with the Status code? There might be more places where we call it.

hi @sttts I try to modify responsewriters.Forbidden, but I think that would make it almost like responsewriters.ErrorNegotiated, so do you think we can keep it ? I checked, and there's no other places invoke responsewriters.Forbidden

There is this msg := sanitizer.Replace(forbiddenMessage(attributes)) line. We should preserve that. How do the error messages differ now from the old message?

sttts · 2017-06-14T08:45:35Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go

@@ -76,24 +98,6 @@ func WithAuthentication(handler http.Handler, mapper genericapirequest.RequestCo
 	)
 }

-func Unauthorized(supportsBasicAuth bool) http.HandlerFunc {


what about adding the mapper and serializer to this function? Then we can keep the (now failing) unit tests.

updated, how do you think about now ?

sttts · 2017-06-14T14:40:32Z

test/integration/master/master_test.go

+			t.Errorf("unexpected status %q, expected %q", got, expected)
+		}
+		if got, expected := decodedData["code"], float64(tc.statusCode); got != expected {
+			t.Errorf("unexpected code %v, expected %v", got, expected)


can we also test reason and message?

shiywang · 2017-06-15T14:33:12Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

sttts · 2017-07-03T06:44:47Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/errors_test.go

@@ -64,18 +67,20 @@ func TestForbidden(t *testing.T) {
 		attributes authorizer.Attributes
 		reason     string
 	}{
-		{`User "NAME" cannot GET path "/whatever".`,
+		{"{\"metadata\":{},\"status\":\"Failure\",\"message\":\" \\\"\\\" is forbidden: User \\\"NAME\\\" cannot GET path \\\"/whatever\\\".\",\"reason\":\"Forbidden\",\"details\":{},\"code\":403}\n",


With backticks you can make this more readible.

sttts · 2017-07-03T06:47:09Z

test/integration/master/master_test.go

+			statusCode:   http.StatusForbidden,
+			reqPath:      "/apis",
+			reason:       "Forbidden",
+			message:      " \"\" is forbidden: User \"\" cannot get path \"/apis\".: \"Everything is forbidden.\"",


sttts · 2017-07-03T07:00:17Z

Looks good overall.

/cc @k8s-mirror-api-machinery-api-reviews please take a look that this is what we want.

@smarterclayton @liggitt do you see any issues with existing clients? This changes how errors are returned. Also: do we disclose anything with these more verbose 403 errors that we don't want to?

shiywang · 2017-07-03T13:28:51Z

/test pull-kubernetes-unit

smarterclayton · 2017-07-03T20:02:15Z

I think that because we are returning more structured errors, and there was no guarantee that another proxy couldn't be introduced (such as api aggregation), anyone expecting a generic error was already broken. In practice, most people are either gracefully handling our structured error (but would be surprised if we returned a generic error), can handle both, or check status code first and everything else is gravy.

So I don't think this has an issue from API compat that (for instance) changing 200 -> 201 did.

smarterclayton · 2017-07-03T20:04:00Z

It looks like this handles negotiation properly, which was my primary concern (took a while in 1.5 to get that sorted). So at first glance I don't see anything concerning.

sttts · 2017-07-04T06:48:18Z

@shiywang please squash

sttts · 2017-07-04T06:48:25Z

/approve

sttts · 2017-07-04T08:08:38Z

/test pull-kubernetes-federation-e2e-gce

shiywang · 2017-07-05T07:25:43Z

@sttts ok to merge ?

sttts · 2017-07-05T08:30:01Z

/lgtm

k8s-github-robot · 2017-07-05T08:30:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shiywang, sttts

Associated issue: 45970

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]
~~test/OWNERS~~ [sttts]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

shiywang · 2017-07-05T09:17:04Z

/test pull-kubernetes-unit

k8s-github-robot · 2017-07-05T09:18:21Z

Automatic merge from submit-queue (batch tested with PRs 48383, 47384)

kubernetes#47384 makes 403 errors return Status Object. How the Content-Type is still "text/plain" This change fix it.

Automatic merge from submit-queue Fix invalid Content-Type for 403 error #47384 makes 403 errors return Status Object. However the Content-Type is still "text/plain" This change fixes it. Before this change: kubectl get pods --as=tom Error from server (Forbidden): {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"pods \"\" is forbidden: User \"tom\" cannot list pods in the namespace \"default\".","reason":"Forbidden","details":{"kind":"pods"},"code":403} (get pods) After this change: $ kubectl get pods --as=tom Error from server (Forbidden): pods "" is forbidden: User "tom" cannot list pods in the namespace "default". **What this PR does / why we need it**: **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: ``` NONE ```

kubernetes/kubernetes#47384 makes 403 errors return Status Object. How the Content-Type is still "text/plain" This change fix it. Kubernetes-commit: 36e0a5ed14ae0fb9fd88980f0fce57d076216e2e

kubernetes#47384 makes 403 errors return Status Object. How the Content-Type is still "text/plain" This change fix it.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 13, 2017

k8s-github-robot assigned timothysc and sttts Jun 13, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Jun 13, 2017

shiywang force-pushed the api403 branch from 1e8db94 to 0a644d5 Compare June 13, 2017 00:30

shiywang changed the title ~~Fix 401/403 apiserver errors do not return 'Status' objects~~ [WIP] Fix 401/403 apiserver errors do not return 'Status' objects Jun 13, 2017

shiywang force-pushed the api403 branch from 0a644d5 to 9c5e3b5 Compare June 13, 2017 01:37

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 13, 2017

shiywang force-pushed the api403 branch 2 times, most recently from 6a6b608 to 7e12adc Compare June 13, 2017 06:37

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 14, 2017

shiywang force-pushed the api403 branch from bb2c9b7 to 0025b07 Compare June 14, 2017 07:10

shiywang changed the title ~~[WIP] Fix 401/403 apiserver errors do not return 'Status' objects~~ Fix 401/403 apiserver errors do not return 'Status' objects Jun 14, 2017

sttts reviewed Jun 14, 2017

View reviewed changes

shiywang force-pushed the api403 branch 3 times, most recently from 1dd2069 to dc0fef5 Compare June 14, 2017 13:50

sttts reviewed Jun 14, 2017

View reviewed changes

shiywang force-pushed the api403 branch 4 times, most recently from 0abef4a to d319f7a Compare June 15, 2017 13:42

sttts reviewed Jul 3, 2017

View reviewed changes

sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jul 4, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 4, 2017

Fix 401/403 apiserver errors do not return 'Status' objects

3d6479f

shiywang force-pushed the api403 branch from eb1d5d0 to 3d6479f Compare July 4, 2017 07:36

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2017

k8s-github-robot merged commit c746680 into kubernetes:master Jul 5, 2017

shiywang deleted the api403 branch July 5, 2017 09:44

CaoShuFeng mentioned this pull request Jul 6, 2017

Fix invalid Content-Type for 403 error #48536

Merged

CaoShuFeng added a commit to CaoShuFeng/kubernetes that referenced this pull request Jul 6, 2017

Fix invalid Content-Type for 403 error

36e0a5e

kubernetes#47384 makes 403 errors return Status Object. How the Content-Type is still "text/plain" This change fix it.

RemingtonReackhof pushed a commit to RemingtonReackhof/kubernetes that referenced this pull request Jul 11, 2017

Fix invalid Content-Type for 403 error

6099f5c

kubernetes#47384 makes 403 errors return Status Object. How the Content-Type is still "text/plain" This change fix it.

enj mentioned this pull request Oct 16, 2017

Loss of API status on Forbidden openshift/origin#16731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 401/403 apiserver errors do not return 'Status' objects #47384

Fix 401/403 apiserver errors do not return 'Status' objects #47384

shiywang commented Jun 13, 2017

sttts commented Jun 13, 2017

shiywang commented Jun 14, 2017 •

edited

sttts Jun 14, 2017

sttts Jun 14, 2017

shiywang Jun 14, 2017

sttts Jun 14, 2017

sttts Jun 14, 2017

shiywang Jun 14, 2017

sttts Jun 14, 2017

sttts Jun 14, 2017

shiywang Jun 15, 2017

shiywang commented Jun 15, 2017

sttts Jul 3, 2017

shiywang Jul 3, 2017

sttts Jul 3, 2017

shiywang Jul 3, 2017

sttts commented Jul 3, 2017

shiywang commented Jul 3, 2017

smarterclayton commented Jul 3, 2017

smarterclayton commented Jul 3, 2017

sttts commented Jul 4, 2017

sttts commented Jul 4, 2017

sttts commented Jul 4, 2017

shiywang commented Jul 5, 2017

sttts commented Jul 5, 2017

k8s-github-robot commented Jul 5, 2017

shiywang commented Jul 5, 2017

k8s-github-robot commented Jul 5, 2017

		glog.V(4).Infof("Forbidden: %#v, Reason: %q", req.RequestURI, reason)
		responsewriters.Forbidden(attributes, w, req, reason)

Fix 401/403 apiserver errors do not return 'Status' objects #47384

Fix 401/403 apiserver errors do not return 'Status' objects #47384

Conversation

shiywang commented Jun 13, 2017

sttts commented Jun 13, 2017

shiywang commented Jun 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiywang commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jul 3, 2017

shiywang commented Jul 3, 2017

smarterclayton commented Jul 3, 2017

smarterclayton commented Jul 3, 2017

sttts commented Jul 4, 2017

sttts commented Jul 4, 2017

sttts commented Jul 4, 2017

shiywang commented Jul 5, 2017

sttts commented Jul 5, 2017

k8s-github-robot commented Jul 5, 2017

shiywang commented Jul 5, 2017

k8s-github-robot commented Jul 5, 2017

shiywang commented Jun 14, 2017 •

edited